[TV] Search: Podcasts/Episodes scope filter - #5729

@claude please review this

Claude finished @sztomek's task in 2m 33s —— View job

Code review: TV Search Podcasts/Episodes scope filter

• Gather context (diff vs feat/tv-search-results)
• Review TvSearchFilters.kt, TvSearchScreen.kt, TvSearchViewModel.kt, TvPodcastGridScaffold.kt
• Post review

Nice, focused change — the scope switching reads cleanly and splitting TvSearchResults into TvSearchTopResults / TvSearchEpisodeList was the right call. A few things worth looking at before merge, in rough priority order.

1. Focus is lost in the Episodes scope after the podcast overlay closes

restoreFocusTrigger exists precisely because tvFocusInactiveWhen(podcastUuid != null) (TvSearchScreen.kt:108) strips focus from the content while the detail overlay is up, and onHide (:113) bumps the trigger so something below re-claims it.

TvSearchTopResults consumes it (:329) and TvPodcastGridScaffold consumes it (:290), but TvSearchEpisodeList (:368-385) doesn't take it at all. Repro on the Episodes tab: focus an episode → open actions → Go to podcast → back out of the podcast detail. The overlay closes, nothing requests focus, and the D-pad goes dead until the user finds another way in.

Related, milder: in TvSearchTopResults the restore always targets podcastsRowFocusRequester, which is only attached when podcasts.isNotEmpty() (:338). Episode-only results hit the same dead-end — runCatching (:333) swallows it silently. Worth a fallback to the first episode row.

Fix this →

2. No explicit focus requester on the selected pill

TvTabBar pins focus to the selected tab (TvTabBar.kt:107, .then(if (index == selectedTabIndex) Modifier.focusRequester(focusRequester) else Modifier)). TvSearchFilterPills relies on focusRestorer() alone.

Since selection follows focus here, any case where the restorer's saved child is dropped means focus lands on Top Results and the whole content region silently swaps out from under the user. Worth exercising: select Episodes → D-pad down into the list → scroll a few items → D-pad up back to the pills. If focus doesn't return to Episodes, the scope resets.

3. title = "" as a "no title" sentinel

TvSearchScreen.kt:287 passes an empty string, and TvPodcastGridScaffold.kt:41 now branches on title.isNotEmpty(). An empty-string sentinel is easy to trip over — title: String? = null expresses it directly and lets the search call site just omit the argument. (The other two call sites, TvFolderDetailScreen.kt:98 and TvYourPodcastsScreen.kt:162, pass real titles and are unaffected.)

Fix this →

4. Podcasts grid is misaligned with the rest of the search screen

The search field, filter row and Top Results carousel all sit at 48dp (ContentPadding, TvSearchScreen.kt:63), but TvPodcastGridScaffold hardcodes contentPadding = PaddingValues(start = 32.dp, …, end = 32.dp) (TvPodcastGridScaffold.kt:77). Switching Top Results → Podcasts shifts the covers 16dp left relative to the pills above them. Either plumb a contentPadding parameter through the scaffold or wrap the grid — worth a quick check against the Figma.

5. Duplicated pill-bar implementation

TvSearchFilterPills (TvSearchFilters.kt:52-108) is a near-verbatim copy of TvTabBar.kt:79-157 — same Box + backgroundSunken + RoundedCornerShape(percent = 50) + 3.dp padding, same TabRow/PillIndicator config, same TabDefaults.pillIndicatorTabColors five-colour block, same 44.dp/21.dp metrics, same inner Box + caption1 Text. That's the styling contract for TV pills duplicated in two files, so any design tweak now has to land in both.

Extracting a shared TvPillTabRow(labels, selectedIndex, onSelect, …) into component/ would let both call sites keep only their own focus behaviour. Not blocking, but the divergence risk is real given how mechanically identical these are.

6. Filter shown for Searching / Error / NoResults

searchState !is TvSearchState.Idle (TvSearchScreen.kt:170) also covers the loading spinner, the error message and the no-results message. In all three the pills are interactive but change nothing, and on Error/NoResults they're a focusable row leading to identical content. Intentional (avoids the row flickering on every keystroke while Searching), or should it be is TvSearchState.Results?

7. Smaller notes

• Modifier.padding(horizontal = 48.dp) at TvSearchScreen.kt:174 duplicates the ContentPadding constant declared 111 lines above — use Modifier.padding(ContentPadding) so they can't drift.
• The no-results message is written three times (:281-284, :304-307) with identical arguments. A local @Composable fun NoResults() or hoisting it above the when would cut the repetition.
• Episode item rendering is duplicated between TvSearchTopResults (:350-361) and TvSearchEpisodeList (:374-382) — a LazyListScope.tvSearchEpisodeItems(episodes, …) extension, mirroring the existing tvSearchPodcastsRow, would cover both.
• color = LocalContentColor.current on the Text (TvSearchFilters.kt:97) is the default already — harmless, and it matches TvTabBar, so only worth dropping if you extract the shared component.
• Preview coverage: only TvSearchFilter.Podcasts is previewed. A TopResults variant would catch first-pill indicator/rounding issues.

8. Tests

TvSearchViewModelTest is already well built out, and the two new VM behaviours are trivially testable with the existing harness — no new mocks needed:

• onFilterSelected updates filter.
• Clearing the query resets filter to TopResults (TvSearchViewModel.kt:104), which is the non-obvious one and easy to regress.

Fix this →

Verdict: the only thing I'd call blocking is #1 — a dead D-pad on TV is a hard stop for the user. #2 is worth a manual pass on device since it's cheap to verify. Everything else is polish and can land as follow-ups.

Note: I reviewed the diff statically and did not build or run the module.
• branch feat/tv-search-filter

Claude finished @sztomek's task in 3m 51s —— View job

Code review: TV Search Podcasts/Episodes scope filter

• Gather context (diff vs origin/main, 4 commits)
• Review TvSearchFilters.kt, TvSearchScreen.kt, TvSearchViewModel.kt, TvPodcastGridScaffold.kt, TvTabBar.kt, TvSearchEpisodeRow.kt
• Post inline comments + summary

Re-reviewed after the three follow-up commits. The two things I called out last time as the real risks are both fixed: TvSearchEpisodeGrid now takes restoreFocusTrigger and restores through the grid's onEnter (TvSearchScreen.kt:403-410), and TvSearchTopResults falls back to the first episode row when there are no podcasts (:382) — so the dead-D-pad-after-overlay case is closed on every scope. The isInitialComposition guard is duplicated three times now but each copy is correct, and the comment explaining why it exists (TvPodcastGridScaffold.kt:63-64) is the kind of thing that saves the next person an hour.

I've left five inline comments. Summary below, most-significant first.

1. Episodes-scope grid drops the episode-actions affordance (inline)

The new two-column grid reaches onOpenEpisodeActions only through onLongClick (TvSearchScreen.kt:436). Every other episode list in the TV app routes through TvEpisodeListItemContainer, which animates a visible TvMoreButton in on focus — including TvSearchEpisodeRow, so the Top Results scope shows that button for the very same episodes.

The result is that narrowing to the scope dedicated to episodes is what makes the episode actions invisible. A held D-pad centre isn't something a TV user discovers, and there's no on-screen hint. This is the one item I'd want resolved before merge; everything else below is polish.

Secondary effect worth checking on device: once onLongClick is set, a held centre press stops firing onClick, so a press-and-hold opens the actions modal instead of playing.

2. The three scopes don't share a left edge (inline)

• search field / pills / Top Results carousel — ContentPadding = 48dp (TvSearchScreen.kt:68)
• Episodes grid — 48dp (:417)
• Podcasts grid — 32dp, hardcoded in TvPodcastGridScaffold.kt:77

So Top Results → Podcasts shifts the covers 16dp left of the pills directly above them, and Podcasts → Episodes shifts them back. A contentPadding parameter on the scaffold (defaulting to today's 32dp) keeps the other two call sites untouched.

Same comment covers title = "" (:303) as a "no title" sentinel — String? = null says it directly.

3. The pill bar is now duplicated verbatim, and it already cost something (inline)

TvSearchFilterPills (TvSearchFilters.kt:57-106) is identical to TvTabBar.kt:79-146 apart from one .then(…focusRequester…) line. The evidence that this matters is in this PR: @geekygecko's grey-state feedback required the same two-line colour change in both files in commit b88363a. Extracting a shared TvPillTabRow into component/ is cheap now.

4. The tab-bar colour change is global, not search-local (inline)

TvTabBar is the app-wide top nav, so the selected Home/Podcasts/Search pill now renders grey (backgroundBase #292B2E) whenever the bar doesn't hold focus — on every screen. Contrast against the #161718 container is fine and this is clearly the intended design, but it's a global nav change riding in a search PR: worth a note in the description and a QA pass that the current tab still reads at a glance from couch distance while focus sits down in the content.

5. No tests for the new state (inline covers the adjacent nit)

TvSearchViewModelTest has a working harness and neither new behaviour needs a new mock:

• onFilterSelected updates filter.
• Clearing the query resets filter to TopResults (TvSearchViewModel.kt:104) — the non-obvious one, invisible from the call site, and exactly the kind of thing a later refactor drops silently.

Add these →

6. Smaller notes

• Modifier.padding(horizontal = 48.dp) (:188) re-states ContentPadding from :68.
• Filter row shows for Searching / Error / NoResults (:184) — interactive but inert in all three. Probably deliberate to avoid flicker while Searching; a one-line comment would settle it.
• The no-results message is written three times (:216-219, :297-300, :320-323) with identical arguments — a local @Composable fun NoResults() collapses it.
• Preview coverage — TvSearchFilters only previews Podcasts (the middle pill). A TopResults variant would catch first-pill indicator/rounding issues, and neither new scope (podcast grid, episode grid) has a preview, so the PR checklist's "any jetpack compose components I added are covered by compose previews" isn't quite met.
• color = LocalContentColor.current (TvSearchFilters.kt:99) is already the default — only worth dropping as part of the extraction in Updated the release version number #3.
• No explicit focus requester on the selected pill. TvTabBar pins focus to the selected tab (:107); TvSearchFilterPills relies on focusRestorer() alone. Since selection follows focus, any case where the restorer's saved child is dropped silently resets the scope to Top Results. Cheap to verify: Episodes → down into the grid → scroll a few rows → back up to the pills.

Verdict: #1 is the one I'd fix before merge — it's a real affordance regression inside the feature this PR adds, and it contradicts the same screen one pill over. #2 is a visible 16dp jump that a designer will file anyway. The rest can land as follow-ups.

Reviewed the diff statically — I did not build the module or run it on a device, so the focus-behaviour notes in #6 are reasoning from the code, not observed.
• branch feat/tv-search-filter

Claude finished @sztomek's task in 3m 59s —— View job

Code review: TV Search Podcasts/Episodes scope filter

• Gather context (diff vs origin/main)
• Review TvSearchScreen.kt, TvSearchFilters.kt, TvSearchViewModel.kt, TvSearchEpisodeRow.kt, TvPodcastGridScaffold.kt, TvTabBar.kt
• Post inline comment + summary

Re-reviewed after the amend to 4cdc38e. The only substantive change since my last pass is in TvSearchContent — the root focusGroup() + focusProperties { onEnter = … } was replaced by a LaunchedEffect(Unit) { requestFocus() }.

1. The new focus-entry mechanism (inline)

The intent is right — the old onEnter fired on every re-entry and was fighting restoreFocusTrigger when the podcast overlay closed. Two follow-ons:

• LaunchedEffect(Unit) + requestFocus() is the racy variant. TvEmptyState.kt:41-44 guards the identical call with withFrameNanos {}, and TvPodcastGridScaffold.kt:55-58 waits on snapshotFlow { …visibleItemsInfo.isNotEmpty() } — this repo already ran into it. Without a guard, runCatching silently swallows the throw and the Search tab opens with nothing focused, i.e. a dead D-pad. Intermittent, so it won't show up reliably in manual QA. One line to fix.
• tvFocusInactiveWhen lost its focus target. It's focusProperties { canFocus = false } applied to the modifier this Column receives (:114). It used to sit on the .focusGroup() target, which deactivated the whole subtree as the KDoc promises; with the group gone it propagates only as far as the nearest focus targets below. This was the only tvFocusInactiveWhen call site with an explicit focusGroup(). Reasoning from the modifier chain, not from a device — worth confirming that with the detail overlay open the D-pad can't reach the results underneath in all three scopes.

2. Still open from the previous pass

Unchanged in this push, so the earlier inline threads still apply:

• Episode actions are long-press-only in the Episodes scope (thread) — TvSearchEpisodeCard in the grid reaches onOpenEpisodeActions only via onLongClick (:432), with no visible affordance. The same episodes in Top Results go through TvEpisodeListItemContainer and show a TvMoreButton on focus. So narrowing to the episode-dedicated scope is what removes the discoverable action. Still the one item I'd resolve before merge; at minimum pass an onLongClickLabel for TalkBack.
• Scopes don't share a left edge (thread) — field/pills/carousel and the episodes grid at 48dp, TvPodcastGridScaffold hardcoded to 32dp (TvPodcastGridScaffold.kt:77). A visible 16dp jump switching Top Results → Podcasts. Same thread covers title = "" (:299) as a "no title" sentinel.
• TvSearchFilterPills duplicates TvTabBar (thread) — the grey-state feedback already had to be applied twice.
• Global tab-bar colour change (thread) — TvTabBar.kt:94/110 affects every screen, worth a line in the description.
• Modifier.padding(horizontal = 48.dp) (:184) still re-states ContentPadding (:68); filter row still renders for Searching/Error/NoResults (thread).

3. Tests

TvSearchViewModelTest has 16 tests and no new mocks are needed for either new behaviour — neither is covered:

• onFilterSelected updates filter.
• Clearing the query resets filter to TopResults (TvSearchViewModel.kt:104) — invisible from the call site and exactly what a later refactor drops silently.

Add these →

4. Smaller notes

• Preview coverage — TvSearchFilters only previews the middle pill (Podcasts); neither new scope (podcast grid, episode grid) has a preview.
• The no-results message is written three times with identical arguments (:212-215, :293-296, :316-319).
• No explicit focus requester on the selected pill, unlike TvTabBar.kt:107. Since selection follows focus, a dropped focusRestorer() entry silently resets the scope to Top Results — cheap to check: Episodes → down into the grid → scroll → back up.
• Scope-specific empty state: with the Episodes pill selected and a query returning only podcasts, the user sees a bare "no results" while Top Results has content. Correct per scope, but the copy doesn't say which scope is empty.

Verdict: the withFrameNanos {} guard in #1 is the cheapest real fix here, and the episode-actions affordance (#2) is still the one behavioural regression inside the feature this PR adds. Everything else is polish.

Reviewed the diff statically — I did not build the module or run it on a device, so the focus-behaviour notes are reasoning from the modifier chains, not observed.
• branch feat/tv-search-filter

@geekygecko thanks for the feedback, valid points!
updated the pills so they now turn greyish when they are selected but not focused

Claude finished @sztomek's task in 4m 6s —— View job

Code review: TV Search Podcasts/Episodes scope filter

• Gather context (diff vs origin/main, changes in eb644a0)
• Review TvSearchScreen.kt, TvSearchFilters.kt, TvPodcastGridScaffold.kt, TvSearchEpisodeRow.kt, TvSearchViewModel.kt + test
• Post inline comment + summary

Re-reviewed after eb644a0. Most of what I raised last pass is now closed:

Also confirmed the TvPodcastGridScaffold signature reorder is safe — both other call sites (TvYourPodcastsScreen.kt:162, TvFolderDetailScreen.kt:98) pass every argument by name.

Retracting one item from my last pass: I flagged that dropping .focusGroup() from TvSearchContent left tvFocusInactiveWhen without a focus target. Having traced the modifier chain properly, that was wrong in direction — focusProperties propagates down to the nearest descendant focus targets, so the two grids' own focusGroup()s now receive canFocus = false exactly as the removed root group did, and the Top Results list gets it on the item targets instead. Same coverage or better. No action needed.

1. Still the one behavioural gap: episode actions are long-press-only in the Episodes scope (thread)

TvSearchEpisodeCard in the grid reaches onOpenEpisodeActions only through onLongClick (:435), with nothing on screen hinting at it. The same episodes one pill over go through TvEpisodeListItemContainer (TvEpisodeListItem.kt:56-84), which animates a visible TvMoreButton in on focus. So narrowing to the scope dedicated to episodes is what removes the discoverable action.

If the grid cell can't spare the horizontal room for the more button, an overflow glyph in the focused card would do. Either way, onLongClickLabel is a one-word addition and gets TalkBack announcing it. Worth confirming on device too that a held centre press now opens the modal instead of playing, since onLongClick suppresses onClick on a long press.

2. Episodes grid padding (inline)

:416 is the last hardcoded 48.dp now that the constant exists two screens away, and unlike the podcast grid it has no top padding — so the first row sits 16dp higher in Episodes than in Podcasts, and the focused card's 1.02f scale gets clipped at the grid's top edge. Suggestion in the thread.

3. TvSearchFilterPills still duplicates TvTabBar (thread)

TvSearchFilters.kt:57-106 vs TvTabBar.kt:79-146: still identical apart from the one .then(…focusRequester…) line, and this push is the second time the same colour change had to be applied to both files. A shared TvPillTabRow in component/ is cheap now and gets less cheap with every tweak. Non-blocking.

4. Smaller, unchanged

• Filter row renders for Searching / Error / NoResults (:183). I now think this is right — it's what keeps the row from flickering out between keystrokes as Results → Searching → Results — but on Error/NoResults it's a focusable row where every pill shows the same content. A one-line comment would settle the intent.
• No-results message written three times with identical arguments (:215-218, :296-299, :319-322). A local @Composable fun NoResults() collapses it.
• Scope-specific empty copy: Episodes selected + a query that returns only podcasts gives a bare "no results" while Top Results has content. Correct per scope, but the copy doesn't say which scope is empty.
• Preview coverage: TvSearchFilters previews only the middle pill (Podcasts) — a TopResults variant would catch first-pill indicator/rounding — and neither new scope (podcast grid, episode grid) has a preview, so the checklist item isn't quite met.
• No explicit focus requester on the selected pill, unlike TvTabBar.kt:107. Since selection follows focus, a dropped focusRestorer() entry silently resets the scope to Top Results. Cheap to check on device: Episodes → down into the grid → scroll a few rows → back up to the pills.
• Global tab-bar colour change (thread) — TvTabBar.kt:94/110 lands on every screen. Requested by @geekygecko and already approved, so just worth a line in the PR description for whoever reads the history.

Verdict: this push cleared everything I'd called blocking or near-blocking except the episode-actions affordance (#1), which is still the one place the feature contradicts itself between scopes. #2 is a two-line fix with a suggestion attached; #3 and #4 are follow-up material.

Reviewed statically — I did not build the module or run it on a device, so the focus-behaviour notes are read off the modifier chains rather than observed.
• branch feat/tv-search-filter